backupccl: add deprecation notice for BACKUP TO and RESTORE FROM without subdir
#78165
Conversation
|
This change is |
| @@ -1075,6 +1075,13 @@ func restorePlanHook( | |||
| if err != nil { | |||
| return nil, nil, nil, false, err | |||
| } | |||
| } else { | |||
There was a problem hiding this comment.
Before the 22.1 release, I think we'll need to add a docs page that provides step by step guidance on restoring backups with the new restore syntax that were originally created with the old backup syntax. That page should be linked to this warning.
There was a problem hiding this comment.
I like the idea of a section about this, but I think it can either be inline where this link points to today as a notice/warning, or be linked out from there. cc: @kathancox
There was a problem hiding this comment.
fwiw, It seems like inline would be a good idea. But, I also think expanding the deprecation notice and recording this in the docs — especially if workarounds are needed if a backup was taken with BACKUP TO, but would need to be restored with the new RESTORE syntax is important. When the docs issue comes through for this with the release notes, I'll prioritize into 22.1 work.
There was a problem hiding this comment.
I don't know if we need workarounds: recall RESTORE only supports restoring backups from the previous major version or newer, so if 22.1 deprecates backup to (but can still restore backups made with it, including by 21.2), then 22.2 can just drop support and we don't need to supply workarounds, since any 22.1-made backups, the oldest it should need to restore, should be in collections anyway.
There was a problem hiding this comment.
@adityamaru do you know if there's a clean migration from old backups that use INCREMENTAL FROM to the new incremental_location parameter? And how that interacts with locality aware backups? Aside from this, I think the user migration is pretty straightforward. Perhaps a data driven test in this PR could validate the different migration stories.
There was a problem hiding this comment.
perhaps then we should make the BACKUP TO warning even scarrier: "your backup will not be restorable in the next major release"
There was a problem hiding this comment.
But it is restorable right?
BACKUP TABLE foo TO 'nodelocal://1/bar' can be restored using RESTORE FROM 'bar' IN 'nodelocal://1/';? The x IN y just appends the path to y/x and looks for the manifest to restore.
I agree that we should have some documentation helping customers in 22.2 restoring backups written in 22.1 using deprecated syntax, but thats docs issue that can be addressed once this is merged?
There was a problem hiding this comment.
yeah, that happy path is restorable, but there may be other user backup file configurations that may not be (e.g. ones that use INCREMENTAL FROM or locality aware backups). But i'm not sure.
There was a problem hiding this comment.
But it is restorable right?
technically yes, but I'd just as soon sidestep this whole mixing old and new and just say it isn't.
There was a problem hiding this comment.
Added a slightly sterner warning to the backup code, let me know what yall think.
adityamaru
force-pushed
the
deprecate-old-backup
branch
2 times, most recently
from
24dd610 to
df1a34a
Compare
Informs: cockroachdb#78153 Release note (sql change): The `BACKUP TO` syntax to take backups is deprecated, and will be removed in a future release. Users are recommended to create a backup collection using the `BACKUP INTO` syntax in our docs.
Informs: cockroachdb#78153 Release note (sql change): The `RESTORE FROM` syntax without an explicit subdirectory pointing to a backup in a collection is deprecated, and will be removed in a future release. Users are recommended to use `RESTORE FROM <backup> IN <collection>` to restore a particular backup in a collection.
Informs: cockroachdb#78153 Release note: None
adityamaru
force-pushed
the
deprecate-old-backup
branch
from
df1a34a to
548c5e4
Compare
|
Thanks for the review! bors r=msbutler |
|
Build succeeded: |
backupccl: add deprecation notice for BACKUP TO
Informs: #78153
Release note (sql change): The
BACKUP TOsyntaxto take backups is deprecated, and will be removed in a future
release. Users are recommended to create a backup collection
using the
BACKUP INTOsyntax in our docs.backupccl: add deprecation notice to RESTORE FROM without subdir
Informs: #78153
Release note (sql change): The
RESTORE FROMsyntax without anexplicit subdirectory pointing to a backup in a collection is deprecated,
and will be removed in a future release.
Users are recommended to use
RESTORE FROM <backup> IN <collection>torestore a particular backup in a collection.
backupccl: add datadriven tests for deprecation notice
Informs: #78153
Release note: None